/hold
we need to wait for 4.1 working branch.

@abhinavdahiya why do we need to wait for a 4.1 branch? this is for 4.0 (exception approved)

We need to do this in Go so that we are using these AMIs for our machine objects.

@abhinavdahiya why do we need to wait for a 4.1 branch? this is for 4.0 (exception approved)

feel free to cancel the hold, I was not aware we were merging 4.0 exception approved feature.

We need to do this in Go so that we are using these AMIs for our machine objects.

It's not clear to me how this is supposed to work with machine objects, which is why I don't think we need to do this in Go (yet). I expect we'll end up with a machine-API operator that can perform these copy-and-encrypt operations on its own, in which case the configuration would still use the source AMI (which is what we're currently putting there now) with an additional property to request "please use an encrypted copy of this AMI" (which doesn't exist yet). I don't think it's a problem to perform this copy-and-encrypt in Terraform while we work out how in-cluster encrypted AMIs are going to work. Thoughts?

We need to do this in Go so that we are using these AMIs for our machine objects.

It's not clear to me how this is supposed to work with machine objects, which is why I don't think we need to do this in Go (yet). I expect we'll end up with a machine-API operator that can perform these copy-and-encrypt operations on its own, in which case the configuration would still use the source AMI (which is what we're currently putting there now) with an additional property to request "please use an encrypted copy of this AMI" (which doesn't exist yet). I don't think it's a problem to perform this copy-and-encrypt in Terraform while we work out how in-cluster encrypted AMIs are going to work. Thoughts?

Why can't the installer create encrypted AMI and set them on machine objects?

Why can't the installer create encrypted AMI and set them on machine objects?

We can, but then we'd need to write metadata.json earlier so callers could clean up the resources (in this case, deregistering the copied AMI) if they later decided not to create the cluster or crashed for some other reason before creating the cluster. But maybe there's no way to maintain our current separation between creating all the manifests and creating external resources? If we decide that we can break that separation, it would mean we could transition from filters to zone IDs for the private hosted zone and similar as well, which would be something of a consolation prize :p.

I've pushed f08e266 with a rebase onto master and destroy cluster AMI deregistration, which is enough to remove the WIP. It would still be nice to deregister during bootstrap teardown, but I think we can circle back to that in follow-up work.

Can we update docs on AWS regarding the AMI creation and check for AMI limits per account.. ?

Also the openshift-dev account will need to prune these when terminating clusters after 48hrs.

Can we update docs on AWS regarding the AMI creation and check for AMI limits per account.. ?

I don't see a limit on the number of AMIs in an account (although maybe I'm just not looking in the right places?). There is "Destination Regions are limited to 50 concurrent AMI copies", although I'm not sure what that means. Is it a limit on concurrent copy operations or is it a cap on the number of AMIs which were originally created as copies of other AMIs? Is it per-account or across all accounts in a given region?

Also the openshift-dev account will need to prune these when terminating clusters after 48hrs.

Can we update docs on AWS regarding the AMI creation and check for AMI limits per account.. ?

I don't see a limit on the number of AMIs in an account (although maybe I'm just not looking in the right places?). There is "[Destination Regions are limited to 50 concurrent AMI copies]", although I'm not sure what that means. Is it a limit on concurrent copy operations or is it a cap on the number of AMIs which were originally created as copies of other AMIs? Is it per-account or across all accounts in a given region?

Can we update docs on AWS regarding the AMI creation

At least this.

Also the openshift-dev account will need to prune these when terminating clusters after 48hrs.

This would be addressed by DPP-1066, although I'm sure we can address this via more minor updates to their reaper in the meantime. I'll get something up there within 48 hours of this landing, although I don't expect this to be time-critical, since I can't find a clear AMI limit that leakers would exhaust.

Can we update docs on AWS regarding the AMI creation

At least this.

I don't see a good place for this. Maybe here? With an additional sentence like "This creates an encrypted AMI for the bootstrap and control-plane machines. The encrypted AMI is deregistered by destroy cluster." I can add that if you like, or I can add whatever wording you want to whatever doc location you want ;). But obviously create cluster creates resources which are removed by destroy cluster (and possibly also the bootstrap teardown), so personally I don't feel a need to call this out in our docs.

Yeah, add a small section that provides info...

Added with f08e266 -> 4fcef5e, although as I explain in the commit message, it's not clear to me whether users can configure the default EBS encryption key for the target region if they want to use a custom CMK instead of the Amazon-managed default. We may need to expose kms_key_id via the install config in follow-up work as we refine encryption support.

/hold cancel

/lgtm

/retest

Please review the full test history for this PR and help us cut down flakes.

All green :)

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here